Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: simplify journal and restore streamer cancellation #4549

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

BorysTheDev
Copy link
Contributor

@BorysTheDev BorysTheDev commented Feb 3, 2025

fixes: #4284
refactor ExecutionState: now we can have 3 independent states RUN, CANCELLED, ERROR

@BorysTheDev BorysTheDev requested a review from adiholden February 3, 2025 13:00
@BorysTheDev BorysTheDev force-pushed the streamer_refactoring branch 2 times, most recently from 4a3cabc to 3735d47 Compare February 4, 2025 10:39
@kostasrim
Copy link
Contributor

kostasrim commented Feb 4, 2025

@BorysTheDev plz do not force push once a PR has comments. It's impossible to keep track of the logical changes between different commits.

  1. If nobody reviewed -> squash and force push as much as you want
  2. If there are reviews -> squash unpushed local commits but leave the previous ones unchanged

@BorysTheDev BorysTheDev changed the title refactor: simplify journal and restore streamer cancellation refactor: simplify journal and restore streamer cancellation NOT READY FOR REVIEW Feb 4, 2025
@BorysTheDev BorysTheDev removed the request for review from adiholden February 4, 2025 11:25
@BorysTheDev BorysTheDev force-pushed the streamer_refactoring branch 4 times, most recently from c6c3c74 to 5c76061 Compare February 14, 2025 09:23
@BorysTheDev BorysTheDev changed the title refactor: simplify journal and restore streamer cancellation NOT READY FOR REVIEW refactor: simplify journal and restore streamer cancellation Feb 14, 2025

if (last_error_) {
LOG(ERROR) << last_error_.Format();
if (cntx_.IsError()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change of the semantics here.

Before: we sleep either if operation::canceled or if we reported via error ReportError.

Now: we only sleep if there was an error. In other words, we no longer treat cancellation as error which is a mistake.

Cancellation is error and that's why errno exists with: ECANCELED == errc::operation_canceled.

I would like us to keep those semantics for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have cancel_error as well. I don't see any problems if you want create cancel error you call reportCancelError() if you want just cancel the process you cancel the process

const Cancellation* ExecutionState::GetCancellation() const {
return this;
}

void ExecutionState::ReportCancelError() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is still being used (in replication and in other places). The issue here is that after this function is called IsError() is true. This is a bug and the correct values should be false and IsCancelled true.

You do not update the state == Cancelled here but this is also not needed. See my top comment regarding the 3 states you introduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you report an error even if it CancelError the state will be error

GenericError ReportErrorInternal(GenericError&& err);

enum class State { RUN, CANCELLED, ERROR };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need these states. You want to figure out if:

  1. Operation cancelled
  2. Operation cancelled with an error

Cancelled operation is an error. That's why we have ReportCancelcedError which cancels the context with the error ECANCELED. Therefore you don't need any of the 3 states here. All you have to do is:

  bool IsError() const {
    return ec && !IsCancelled();
  }

  bool IsCancelled() const {
    return ec && err_ == ECANCELED;
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your state machine:

RUN == (ec_ == std::error_code{})
CANCELLED == (ec_ == ECANCELED)
ERROR == (ec_ && !CANCELLED) 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you use ec_ you need mutex and it's slow. Let's have a call.

@BorysTheDev BorysTheDev requested a review from romange February 14, 2025 11:56
@@ -84,6 +84,8 @@ class OutgoingMigration::SliceSlotMigration : private ProtocolClient {
}

void Cancel() {
// We don't care about errors during cancel
cntx_.SwitchErrorHandler([](auto ge) {});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other places in code that we do care about error during cancel?
Should this be the caller to decide if we need error handling after cancel was sent or doesnt it makes more sense that ExecutionState will handle this i.e after cancel there is not switching to error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about such places, so I can remove errorHandler in the cntx.Cancel() call

@BorysTheDev BorysTheDev merged commit a22daaf into main Feb 20, 2025
10 checks passed
@BorysTheDev BorysTheDev deleted the streamer_refactoring branch February 20, 2025 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor Journal and Restore streamers
3 participants